fix: prevent channel monitor overwrite in migrations#74
Open
fix: prevent channel monitor overwrite in migrations#74
Conversation
During FS→KV store migration, the code now compares update_id before writing and skips if the KV store already has a newer monitor. This prevents stale migration data from clobbering current channel state on repeated restarts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace incomplete testing docs with comprehensive coverage of unit tests, integration tests (with macOS ulimit note), and CLN/LND/VSS backend tests using correct RUSTFLAGS syntax. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
ovitrif
requested review from
ben-kaufman and
coreyphillips
and removed request for
ben-kaufman and
coreyphillips
Address PR review: change channel monitor migration error handling from fail-open (proceed with overwrite) to fail-closed (abort with ReadFailed) for both deserialization errors and non-NotFound I/O errors. This prevents silent data loss when the KV store has valid data that can't be read due to transient errors or format changes. Also moves CHANGELOG entry to Synonym Fork Additions per project convention. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ovitrif
changed the title
The previous bindgen run used gobley-uniffi-bindgen v0.3.7 (from gobley main branch) instead of v0.2.0 (from fix-v0.2.0 branch), which added explicit `public` visibility modifiers and return types throughout the generated Kotlin code. Regenerated with the correct version. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract the channel data migration block from build_with_store_internal into a standalone apply_channel_data_migration function, making it generic over the keys manager type to enable direct unit testing. Add 7 unit tests covering all code paths: - Invalid monitor data returns ReadFailed - Empty monitors list succeeds - Fresh write to empty store - Equal update_id (existing == migrated) still writes - Existing data with same update_id gets overwritten - Corrupt existing data triggers fail-closed (ReadFailed) - Store IO error triggers fail-closed (ReadFailed) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Looks good, but needs to add some tests ideally. |
Collaborator
|
One nit. Line |
coreyphillips
approved these changes
Mar 11, 2026
Collaborator
Author
Thx, added unit tests, does that suffice?! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
update_idbefore writing, preventing stale data from overwriting current state on repeated migrations/restarts.ReadFailed) instead of fail-open, preventing silent data loss from transient I/O errors or format mismatches.0.7.0-rc.33and regenerated bindings.Test plan
cargo fmt --check— cleancargo build— builds successfullyRelease
🤖 Generated with Claude Code